Skip to content

Conversation

@iarenaza
Copy link
Contributor

@iarenaza iarenaza commented Apr 2, 2025

The proposed change tries to respect other values in the Sec-WebSocket-Protocol header, that the caller may have set in the headers option when calling make-channel-socket-client!

[Re: #418]

@iarenaza
Copy link
Contributor Author

iarenaza commented Apr 2, 2025

I opted for prefixing the actual CSRF token value with sente-csrf-token-, to make it easier to find it and extract it on the server side. One of the reasons for that is that protocol names in the Sec-WebSockel-Protocol header cannot use a bunch of non-alphanumeric characters (like spaces, colons, dashes, etc). And we want the value to be easily recognizable, in case the client wants to send other protocol names in that same header (for some custom extra behavior).

If you feel this is over-engineering the solution, feel free to strip that out.

@ptaoussanis ptaoussanis self-assigned this Apr 3, 2025
@ptaoussanis
Copy link
Member

@iarenaza Thank you Iñaki, this looks good! Will try take a closer look + merge by end of tomorrow 👍

@iarenaza
Copy link
Contributor Author

iarenaza commented Apr 5, 2025

Whoops. I had forgotten the other side of the connection: extracting the CRSF token from the Sec-WebSocket-Protocol header on the server side.

@ptaoussanis
Copy link
Member

@iarenaza Hi Iñaki, sorry for the delay on this.

Before merging, I'd like to step back a moment and re-evaluate what we're trying to do here. The current PR seems to be based on this suggestion made earlier by @kaosko, right?

But it's actually not obvious to me what the motivation is/was behind that suggestion.

@kaosko Sorry for digging this up after so long, but could you maybe explain why you've previously suggested that the Sente CSRF token be sent in the Sec-WebSocket-Protocol header? (Context here). It's not obvious to me what the benefit is, or that the header is intended as a place to convey a CSRF token.

Even though the Protocol header seems to be an application-level header, it seems we'd be abusing the semantic intention of the header no? I imagine that stands a non-zero chance of causing issues in the future, and seems like it anyway could be surprising. What's the anticipated upside?

@kaosko
Copy link

kaosko commented Apr 7, 2025

It's a JavaScript spec/API thing. It doesn't allow you to set any other headers for the WebSocket request object than "Sec-WebSocket-Protocol" so if you want to pass auth/CSRF tokens in the headers, that's the way to do it. I wouldn't call it abusing the semantic intention, perhaps merely stretching it, again as an application level protocol. In the grand scheme of things, I guess not that different from manufacturer specific Wi-Fi Information Elements or DHCP Vendor Options.

Stepping back, the anticipated upside according to OWASP (the most authoritative I could find) is that "A CSRF token must not be leaked in the server logs or in the URL".

Almost anything has a non-zero chance of causing issues in the future, but the server is certainly free to ignore any values in Sec-WebSocket-Protocol that it doesn't understand. Plenty of web pages, frameworks still sending various "X-*" headers while the naming practice was deprecated in 2012 and the servers may or may not act on those (and vice versa). That said, I'm not strongly arguing for adding this but don't see how it could hurt much either. Finally, I have used and still use the Sec-WebSocket-Protocol header for sending authentication tokens with the websocket request.

@iarenaza
Copy link
Contributor Author

iarenaza commented Apr 7, 2025

Yeah, exactly what @kaosko said. In any case, for us this is the only acceptable way. We develop SPAs that use OpenID Connect ID tokens to authenticate the user. We have no sessions, use no cookies, etc. So having a CSRF token generated on the backend and sent to the frontend is not something that we can do that easily.

We already have tokens that are short lived (the ID tokens) and cannot be stolen via JS, or automatically sent by the browser on your behalf (you need to explicitly add them in the requests, as a Bearer token). So we are leveraging those tokens for Sente CSRF purposes too.

But passing those tokens in the URL as a parameter is not something that we want to do. They leak in the server logs, in any intermediate proxy, etc. That is why we went with the :ws-constructor option: to be able to (ab)use the Sec-WebSocket-Protocol header to sent them, and not leak them as easily. (and that is how we found the :ws-constructor option didn't work in the first place).

ptaoussanis pushed a commit that referenced this pull request Apr 7, 2025
…@iarenaza)

Before this commit:
  WebSocket client conveys CSRF token to server via `:csrf-token` query param.

After this commit:
  WebSocket client conveys CSRF token to server via `Sec-WebSocket-Protocol` header.

Motivation:
  Moving the token from query param to a header helps reduce the likelihood of
  accidental leaking (e.g. via logging).

  While the `Sec-WebSocket-Protocol` header isn't specifically intended for conveying
  metadata like a CSRF token, the consensus seems to be that this is anyway a practical
  choice without major downsides or obviously better alternatives.

As implemented the change tries to respect other values in the `Sec-WebSocket-Protocol`
header that may have been set when calling `make-channel-socket-client!`.
@ptaoussanis
Copy link
Member

Thanks both for the extra info and quick feedback! 🙏

I've just pushed [com.taoensso/sente "1.21.0-alpha1"] to Clojars 👍

@iarenaza
Copy link
Contributor Author

iarenaza commented Apr 8, 2025

@ptaoussanis Just to confirm that 1.21.0-alpha1 works as expected in our case 🎉 . We no longer need to use the :ws-custom option on the client side 🙏

Side comment: we still need to use some of the available "callback functions" (like csrf-token-fn) on the backend side (to deal with server side CSRF token retrieval and validation). But that is totally expected, because of the type of value that we are using as the CSRF token (as I already mentioned, we are using OpenID Connect ID tokens as the CSRF token)

So, thanks a lot @ptaoussanis !

@ptaoussanis
Copy link
Member

Great, thanks for the confirmation @iarenaza!

@ptaoussanis ptaoussanis closed this Apr 8, 2025
@iarenaza
Copy link
Contributor Author

@ptaoussanis We've found that the approach used in this pull request is not enough to work with Google Chrome. We did all our development and testing with Firefox, and it worked great. But when we tried with Chrome, it refused to open the websocket connection.

The problem seems to be that the web server (http-kit 2.8.0 in our case) doesn't send any Sec-WebSocket-Protocol headers back to the client. I tried adding the header myself to the ring response map that we return from our handler (that wraps (:ajax-get-or-ws-handshake-fn chsk)), but apparently it's not taken into account by http-kit.

Firefox is fine with this, and considers the websocket negotiation handshake successfully done. But apparently Chrome doesn't consider it successful, if it doesn't have a Sec-WebSocket-Protocol header in the response from the server (and it apparently has to contain one of the values sent by the client in the request, if our tests are to be believed).

According to RFC 6455, section 1.3 Opening handshake (emphasis mine):

The |Sec-WebSocket-Protocol| request-header field can be used to indicate what subprotocols (application-level protocols layered over the WebSocket Protocol) are acceptable to the client. The server selects one or none of the acceptable protocols and echoes that value in its handshake to indicate that it has selected that protocol.

and later on, when talking about the handshake from the server side:

In this version of the protocol, the main option field is |Sec-WebSocket-Protocol|, which indicates the subprotocol that the server has selected. WebSocket clients verify that the server included one of the values that was specified in the WebSocket client's handshake. A server that speaks multiple subprotocols has to make sure it selects one based on the client's handshake and specifies it in its handshake.

But the whole 1.3 section is marked as non-normative. Which I interpret as not being mandatory to follow (even if it is the recommended way).

Later on, in section 1.9 it says:

The client can request that the server use a specific subprotocol by including the |Sec-WebSocket-Protocol| field in its handshake. If it is specified, the server needs to include the same field and one of the selected subprotocol values in its response for the connection to be established.

It clearly states that the server needs to include the same field and the selected protocol. But then again, the whole 1.9 section ir marked as "non-normative".

Given that sente doesn't control what headers are returned in the server response for the connection upgrade rquest (they are fully in control of http-kit at the moment), I think we need to step back a bit an re-asses our options. And in the mean time, revert this commit to avoid the breakage it introduced. What do you say?

P.S. We are so sorry that we didn't test it with Chrome before submitting this PR and wasted your valuable time 😞

@kaosko
Copy link

kaosko commented Apr 14, 2025

What about Aleph, Undertow etc? It seems that http-kit just returns whatever you stick in :ring.websocket/protocol key of the ring response, no?

@iarenaza
Copy link
Contributor Author

What about Aleph, Undertow etc? It seems that http-kit just returns whatever you stick in :ring.websocket/protocol key of the ring response, no?

I don't know about Aleph, Undertow, and the rest of the community adapters (I haven't used them). But I know that http-kit only does that if you are using the Ring Websocket Protocol library. If you are using the as-channel "interface", which sente uses with the standard http-kit adapter[1][2][3], things work in a completely different way. And that other way is the one that doesn't let you control what headers you want to return in your connection upgrade.

[1] https://github.com/taoensso/sente/blob/master/src/taoensso/sente.cljc#L1001
[2] https://github.com/taoensso/sente/blob/master/src/taoensso/sente/server_adapters/http_kit.clj#L20
[3] https://github.com/taoensso/sente/blob/master/src/taoensso/sente/server_adapters/http_kit.clj#L26

@ptaoussanis
Copy link
Member

ptaoussanis commented Apr 15, 2025

@iarenaza Hi Iñaki, thanks for the detailed+clear update 🙏

P.S. We are so sorry that we didn't test it with Chrome before submitting this PR and wasted your valuable time 😞

No problem at all, I appreciate the time + effort you've put into this! I'm sorry that you've been running into issues with Sente and/or http-kit!

Re: how to proceed- I've unfortunately got limited knowledge + bandwidth on this topic since I'm currently quite deeply buried in a few other things.

I am planning a big batch of Sente work for June, but hopefully we can make progress here in the meantime!

Can we first back up a bit so that I can confirm my understanding?

@iarenaza Your motivation for #465 is described here, right?

So:

  • You already have a value that'd work as a CSRF token.
  • But you don't want the client to send it to the server over a query param since you're concerned about the token leaking / getting logged.

So the goal is to get the CSRF token from the client to the server in a way that:

  1. Doesn't leak the token.
  2. Works reliably across different browsers and HTTP servers.
  3. Minimizes the size/complexity of changes needed in Sente.

Is that right?

If we were to enumerate all the options we have for getting this done- what would that include?

  1. Send the token in a Sec-WebSocket-Protocol header
  2. Send the token in another header? Is that possible?
  3. Send the token in the first WebSocket message? Is that possible?
  4. Any other ideas?

I haven't had the opportunity to properly investigate this myself, so any input here would be very welcome!

IIUC the main impediment to (1) is:

  • Chrome requires a Sec-WebSocket-Protocol response header.
  • At least http-kit doesn't automatically provide this header in its response.
  • At least http-kit doesn't manually allow this header in its response.

Is that right?

So it seems like our options are something like:

  • 1A) Sente fix for (1) that works with all HTTP servers if possible
  • 1B) http-kit fix for (1), and confirm that other HTTP servers also work
  • 2) Instead try to use an alternative header if possible
  • 3) Instead try to use the first WebSocket message if possible
  • 4) Other ideas

Any thoughts on which direction might be simplest? What do other libraries like Sente tend to do?

@ptaoussanis ptaoussanis reopened this May 16, 2025
ptaoussanis added a commit that referenced this pull request May 16, 2025
Temporarily revert commit fbf13c1
since it looks like that change might cause problems with some
browser + server combos (e.g. Chrome + http-kit).
@ptaoussanis
Copy link
Member

Any update on this issue? I'm aiming to cut a new Sente release around the end of this month, so this would be a good time to get in any pending changes.

ptaoussanis pushed a commit that referenced this pull request Jun 25, 2025
…@iarenaza)

Before this commit:
  WebSocket client conveys CSRF token to server via `:csrf-token` query param.

After this commit:
  WebSocket client conveys CSRF token to server via `Sec-WebSocket-Protocol` header.

Motivation:
  Moving the token from query param to a header helps reduce the likelihood of
  accidental leaking (e.g. via logging).

  While the `Sec-WebSocket-Protocol` header isn't specifically intended for conveying
  metadata like a CSRF token, the consensus seems to be that this is anyway a practical
  choice without major downsides or obviously better alternatives.

As implemented the change tries to respect other values in the `Sec-WebSocket-Protocol`
header that may have been set when calling `make-channel-socket-client!`.
ptaoussanis added a commit that referenced this pull request Jun 25, 2025
Temporarily revert commit 8810cd2
since it looks like that change might cause problems with some
browser + server combos (e.g. Chrome + http-kit).
@ptaoussanis ptaoussanis force-pushed the master branch 8 times, most recently from 84e09ed to 6088457 Compare September 5, 2025 09:59
@iarenaza
Copy link
Contributor Author

Sorry for not coming back to you earlier. We've been in a sustained rush at work for a few months 😞

Can we first back up a bit so that I can confirm my understanding?

@iarenaza Your motivation for #465 is described here, right?

So:

  • You already have a value that'd work as a CSRF token.

  • But you don't want the client to send it to the server over a query param since you're concerned about the token leaking / getting logged.

Correct on all counts!

So the goal is to get the CSRF token from the client to the server in a way that:

  1. Doesn't leak the token.

  2. Works reliably across different browsers and HTTP servers.

  3. Minimizes the size/complexity of changes needed in Sente.

Is that right?

Ideally, that would be our goal, yes.

If we were to enumerate all the options we have for getting this done- what would that include?

  1. Send the token in a Sec-WebSocket-Protocol header

  2. Send the token in another header? Is that possible?

As far as I know, there is no other header that you can use with websockets from a web browser (nodejs may be a different kettle of fish). According to Mozilla Developer Network, when you create a websocket you can only specify the URL for the connection end point, and an optional list of sub-protocols that the client would like to negotiate with the server side of the connection.

  1. Send the token in the first WebSocket message? Is that possible?

I guess that would be possibility, but that would mean that the websocket would always be opened, no matter what is sent in the first message. And later, once the server side has checked the token and decided on whether the token is ok or not, either keep the connection opened or close it for good.

This option would also imply that sente would be unusable for those use cases where sub-protocol negotiation is a must. That is certainly not our case. And probably not something that most people try to use, or even know about. So I would say this is not a show stopper (IMHO).

  1. Any other ideas?

Not that I can think of at the moment.

I haven't had the opportunity to properly investigate this myself, so any input here would be very welcome!

IIUC the main impediment to (1) is:

  • Chrome requires a Sec-WebSocket-Protocol response header.

  • At least http-kit doesn't automatically provide this header in its response.

  • At least http-kit doesn't manually allow this header in its response.

Is that right?

As far as I know, that is 100% right. I have a patch set (for both Sente and http-kit) that would:

  • allow Sente to add some extra headers while negotiating the websocket open connection
  • make http-kit respect those headers and send them back in the websocket open connection response.

The patch set is little more than a proof of concept, to see that we could theoretically make it work. But I'm not sure of the unintended side-effects of those changes (I'm not an http-kit expert by any means!).

I could clean it up (it's full of prn to trace the execution of the requests and responses and see where I needed to "patch in" myself to achieve what I wanted) and share it as a gist, if you think it could be useful as a discussion starting point.

So it seems like our options are something like:

  • 1A) Sente fix for (1) that works with all HTTP servers if possible

  • 1B) http-kit fix for (1), and confirm that other HTTP servers also work

  • 2) Instead try to use an alternative header if possible

  • 3) Instead try to use the first WebSocket message if possible

  • 4) Other ideas

Any thoughts on which direction might be simplest? What do other libraries like Sente tend to do?

I haven't looked at other libraries like Sente, so I don't really know. But most of the examples I've seen out there (in a quick search) don't even try to prevent CSRF attacks, or don't use CSRF tokens (and only rely on Origin header checks).

@ptaoussanis ptaoussanis force-pushed the master branch 18 times, most recently from bb88305 to 16fbe7c Compare October 11, 2025 13:46
The proposed change tries to respect other values in the
Sec-WebSocket-Protocol header, that the caller may have set in the
headers option when calling make-channel-socket-client!

Use the pre-ws-handshake option introduced in http-kit commit to Add
the Sec-WebSocket-Protocol response header in the websocket
upgrade. Some browsers (like Google Chrome and Google Chrome derived
ones) fail to complete the upgrade if the upgrade response doesn't
include the Sec-WebSocket-Protocol with one of the sub-protocols
proposed in the request.

[Re: 465]
@iarenaza
Copy link
Contributor Author

Hi @ptaoussanis ,

I have finally extracted my original changes from my local test repos, cleaned them up and removed all the prns, etc. And I have applied those changes on top of the sente and http-kit main/master branches (as of today, 2025.11.26). I have kept the original sente and http-kit versions in their respective project.clj files, to be able to test them more easily using the Leiningen "checkouts" feature.

Then I have created a separate git repo, using the example-project code from the sente project, where I have cloned my forked repositories in the checkouts directory.

This is what I have done:

  1. Clone the https://github.com/iarenaza/sente-example-project.git repository locally.

  2. Create a checkouts directory inside the cloned repository.

  3. Inside the checkouts directory, clone these two other repositories, that contain my patches:

  4. After the previous steps, we should have the following directory structure:

    .
    ├── checkouts
    │   ├── http-kit
    │   └── sente
    ├── resources
    │   └── public
    └── src
        └── example
    
  5. Now we can run lein start or lein start-dev, as in the original example-project from the sente project, from the top of the sente-example-project cloned repository.

  6. I tested it with both Firefox 144.0 and Chromium "142.0.7444.175 (Official Build) built on Debian GNU/Linux 13 (trixie) (64-bit)", both running on Debian Trixie, and it worked as expected.

Do the changes look like something that you could consider including?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants